Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PGN Parser #3

Merged
merged 45 commits into from
Jan 19, 2023
Merged

PGN Parser #3

merged 45 commits into from
Jan 19, 2023

Conversation

nav-28
Copy link
Contributor

@nav-28 nav-28 commented Jan 1, 2023

#1

@nav-28
Copy link
Contributor Author

nav-28 commented Jan 1, 2023

Just saw someone else is also working on this as well

@nav-28 nav-28 marked this pull request as ready for review January 10, 2023 18:41
@nav-28
Copy link
Contributor Author

nav-28 commented Jan 10, 2023

Added benchmarks for the parser. After updating the packages use dart run benchmark to run the file

Copy link
Collaborator

@veloce veloce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed only the public facing API aspect for now; there are a lot of changes to do that should be tested with dart doc comment to eliminate unresolved doc references.
thank you!

lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated
bool operator ==(Object other) {
return other is Comment &&
text == other.text &&
// shapes == other.shapes && List == operator doesnt compare each component of list
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this code is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was used in testing to check if two comment objects were equal. But dart doesn't equate the list even if they have the same elements.
I don't think it's necessary.

1. Renamed classes Node-> PgnNode, ChildNode->PgnChildNode, Game->PgnGame
2. Renamed parsePgn to parseMultiGamePgn
3. Added static methods makePgn and parsePgn to PgnGame
4. Moved makeVariant to its enum and renamed it to Variant.fromPgn
5. Move headers function to PgnGame
6. Removed walk function
7. Made some enums and classes private
8. Moved Outcome function to its class
1. Add makePgn benchmark
2. Renmane Comment and Evaluation Classes
3. Move function into classes
4. update tests
Also renamed functions and added comments
/// SAN representation of the move
final String san;

/// Starting comments of the node
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a starting comment? it's hard to understand without context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand it either but chessops had it so I implemented that

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PGN moves can have a comment before and/or after the move.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
1. Seperate PgnNodeData copyWith into 2 sepearate functions
2. Rename transformStack
3. Change parse function type and make PgnParse private
4. Use stringBuffer in makePgn instead of lists
5. Fix hashCode and == operator for PgnEvaluation
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
lib/src/pgn.dart Outdated Show resolved Hide resolved
test/pgn_test.dart Outdated Show resolved Hide resolved
lib/src/position.dart Outdated Show resolved Hide resolved
lib/src/position.dart Outdated Show resolved Hide resolved
lib/src/position.dart Outdated Show resolved Hide resolved
lib/src/position.dart Outdated Show resolved Hide resolved
lib/src/position.dart Outdated Show resolved Hide resolved
lib/src/position.dart Outdated Show resolved Hide resolved
@veloce veloce merged commit 0b69060 into lichess-org:main Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants